fix(query-core): restore NoInfer on persister's TQueryKey#10601
Open
neefrehman wants to merge 2 commits intoTanStack:mainfrom
Open
fix(query-core): restore NoInfer on persister's TQueryKey#10601neefrehman wants to merge 2 commits intoTanStack:mainfrom
neefrehman wants to merge 2 commits intoTanStack:mainfrom
Conversation
Follow-up to TanStack#10510. That PR removed NoInfer from all three persister generics to fix TQueryFnData inference when the companion queryFn declares a parameter (TanStack#7842). Keeping NoInfer on TQueryKey preserves that fix while preventing the persister slot from widening TQueryKey inference. Without NoInfer on TQueryKey, the persister slot contributes to TQueryKey inference. When Register.queryKey is augmented to a narrowed constraint, TQueryKey widens to that constraint instead of the literal passed to queryKey. Wrappers that brand their return with DataTag<TQueryKey, ...> then produce a brand on the wider type, which a plain literal tuple can no longer satisfy in contravariant positions (vi.mocked(...).mockReturnValue, typed variable assignments, etc.). TQueryFnData still participates in inference, so TanStack#10510's positive and negative type tests continue to pass.
Contributor
📝 WalkthroughWalkthroughThis pull request applies a surgical TypeScript type inference fix to the Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
View your CI Pipeline Execution ↗ for commit 11e5b6a
☁️ Nx Cloud last updated this comment at |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/query-core/src/types.ts (1)
249-249: Add a focused d.ts regression test for this inference path.Non-blocking: the type change looks correct, but this exact
Register.queryKey-augmentation +DataTag+persisterinteraction previously regressed and should be pinned with an isolated type test to prevent reintroduction.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/query-core/src/types.ts` at line 249, Add a focused TypeScript declaration test that pins the inference path involving the persister property and its generics: create a .d.ts or .test-d.ts that reproduces the Register.queryKey augmentation combined with a DataTag and a persister: ensure the persister is typed as QueryPersister<TQueryFnData, NoInfer<TQueryKey>, TPageParam> and assert the inferred types (using type assertions or expect-type utilities) for the resulting query registration so the previously regressed interaction is covered and will fail if inference changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/query-core/src/types.ts`:
- Line 249: Add a focused TypeScript declaration test that pins the inference
path involving the persister property and its generics: create a .d.ts or
.test-d.ts that reproduces the Register.queryKey augmentation combined with a
DataTag and a persister: ensure the persister is typed as
QueryPersister<TQueryFnData, NoInfer<TQueryKey>, TPageParam> and assert the
inferred types (using type assertions or expect-type utilities) for the
resulting query registration so the previously regressed interaction is covered
and will fail if inference changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 993261ce-7bfd-48c9-bd0d-b4dd4fb44e5c
📒 Files selected for processing (2)
.changeset/fix-persister-query-key-infer.mdpackages/query-core/src/types.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up to #10510. That PR removed
NoInferfrom all threepersistergenerics to fixTQueryFnDatainference when the companionqueryFndeclares a parameter (#7842). This restoresNoInfer<TQueryKey>only — keeping #10510's benefit while preventing thepersisterslot from wideningTQueryKeyinference.Fixes #10600.
The interaction
Without
NoInferonTQueryKey, thepersisterslot contributes toTQueryKeyinference. WhenRegister.queryKeyis augmented to a narrowed constraint (as documented and introduced in #8521),TQueryKeywidens to that constraint instead of the literal passed toqueryKey. Wrappers whose return type brands the key withDataTag<TQueryKey, ...>(which is required to support type-safety when using options with e.g.getQueryData) then produce a brand on the wider type, and a plain literal tuple can no longer satisfy both the augmented constraint and the phantom brand.See #10600 for the minimal repro (a ~15-line wrapper + augmented
Register.queryKey+ a typed variable assignment reproducesTS2322on 5.100.5 and compiles cleanly on 5.100.1).Why this fix is safe
TQueryFnDatastill participates in inference, so the positive-inference test from fix(query-core): stop wrapping persister generics in NoInfer #10510 ('should infer TQueryFnData from persister paired with a queryFn declaring a parameter') passes unchanged.@ts-expect-errorstill triggers.test:typesvariants pass (TS 5.4 through 6.0) and all unit tests pass.A note on regression testing
I tried to add a
.test-d.tsxthat augmentsRegister.queryKeyand asserts aDataTag-branded wrapper return accepts literalqueryKeys. It caught the bug, butdeclare module '@tanstack/query-core'is global — the augmentation leaked into every sibling.test-d.tsxand produced ~30 spuriousTS2322s acrossuseQuery,useSuspenseQuery, etc. (all of which use literalstring[]keys that don't satisfy the narrowed constraint).More broadly, I didn't find any existing tests for the
Registeraugmentations (defaultError,queryMeta,mutationMeta,queryKey,mutationKey) — by construction each would need its own isolated compilation unit. Flagging it in case the maintainers want to set that up separately; I've left it out of this PR to keep the scope minimal.Test plan
packages/query-core—pnpm run test:types(TS 5.4 – 6.0): passpackages/react-query—pnpm run test:types(TS 5.4 – 6.0): passpackages/query-core— vitest: 526 pass / 0 failpackages/react-query— vitest: 540 pass / 0 failSummary by CodeRabbit